Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

📖 Update InfraCluster provider contract to account for the paused condition #10519

Merged
merged 1 commit into from
Oct 10, 2024
Merged

📖 Update InfraCluster provider contract to account for the paused condition #10519

merged 1 commit into from
Oct 10, 2024

Conversation

theobarberbany
Copy link
Contributor

@theobarberbany theobarberbany commented Apr 25, 2024 •
edited
Loading

What this PR does / why we need it:
This change updates the infra cluster provider contract to account for a new paused
condition.

Relates #10130

/area documentation

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 25, 2024
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area PR is missing an area label needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 25, 2024
@k8s-ci-robot
Copy link
Contributor

Hi @theobarberbany. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@theobarberbany
Copy link
Contributor Author

/area documentation

@k8s-ci-robot k8s-ci-robot added area/documentation Issues or PRs related to documentation and removed do-not-merge/needs-area PR is missing an area label labels Apr 26, 2024
@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from 285b232 to 2ff8fbd Compare April 26, 2024 11:59
@theobarberbany theobarberbany changed the title WIP: Update provider contract to account for the paused condition Update provider contract to account for the paused condition Apr 26, 2024
@theobarberbany theobarberbany marked this pull request as ready for review April 26, 2024 12:18
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 26, 2024
@theobarberbany theobarberbany mentioned this pull request Apr 26, 2024
Open
@sbueringer sbueringer changed the title Update provider contract to account for the paused condition 📖 Update provider contract to account for the paused condition Apr 29, 2024
@sbueringer
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 29, 2024
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2024
@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from 2ff8fbd to 24a20d0 Compare July 8, 2024 11:02
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2024
@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch 3 times, most recently from 7207fac to 4d6f9c2 Compare July 8, 2024 13:15
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 25, 2024
@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch 2 times, most recently from 5be71d8 to 965cb82 Compare September 25, 2024 17:59
@fabriziopandini
Copy link
Member

@theobarberbany things are moving fast in the CAPI world!

FYI, https://github.com/fabriziopandini/cluster-api/blob/main/docs/proposals/20240916-improve-status-in-CAPI-resources.md recently merged, introducing a paused conditions in all CAPI resources.

Also as a side effect, there is a major refactor of docs in progress on main WRT to provider contract pages (I also gave an update yesterday at the office hours). Work is tracked in #11105 (a few PR are still missing, but I hope to complete this work by end of next week)

It would be great if contract changes in this PR can build on top of it 😅

@theobarberbany
Copy link
Contributor Author

Hey @fabriziopandini,

That's great! I was about to ask, as the Infra Cluster contract looks significantly different to the others!

I'll try and build off of that, as it seems the others will follow it's style (e.g https://github.com/kubernetes-sigs/cluster-api/pull/11223/files) How does that sound? :D

Also is there anything else in #11105 I can do to help?

@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from 965cb82 to b4e6936 Compare September 26, 2024 14:36
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 26, 2024
@theobarberbany theobarberbany changed the title 📖 Update provider contract to account for the paused condition 📖 Update InfraCluster provider contract to account for the paused condition Sep 26, 2024
@theobarberbany
Copy link
Contributor Author

@fabriziopandini I've updated this PR to add the paused condition to the InfraCluster provider contract. Can you let me know if these changes look reasonable? :)

@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from b4e6936 to adfa182 Compare September 27, 2024 13:40
fabriziopandini
fabriziopandini reviewed Sep 30, 2024
View reviewed changes
docs/book/src/developer/providers/contracts/infra-cluster.md Outdated
@@ -308,6 +308,8 @@ If a condition with type `Ready` exist, such condition will be mirrored in Clust
Please note that the `Ready` condition is expected to surface the status of the InfraCluster during its own entire lifecycle,
including initial provisioning, the final deletion process, and the period in between these two moments.

Provider implementers SHOULD implement `Status.Conditions[Paused]` to report if the cluster is paused. It should check if 'spec.paused' is set on the cluster, and for the paused annotation. Implementing this condition is currently optional, but we may revisit this in the future.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would add a dedicated, optional rule for pause stating that

  • providers SHOULD implement the pause behaviour for every object with a reconciliation loop (eg. pause machines when the cluster or the object is paused; pause does not makes sense for templates)
  • if implementing the pause behaviour, providers SHOULD surface the pause status for an object with a condition

Also, let's be specific and name the annotation that trigger pause at object level

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I think I've updated as you've asked! PTAL :)

@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from adfa182 to ccd3d15 Compare October 1, 2024 14:13
fabriziopandini
fabriziopandini reviewed Oct 7, 2024
View reviewed changes
docs/book/src/developer/providers/contracts/infra-cluster.md Outdated Show resolved Hide resolved
docs/book/src/developer/providers/contracts/infra-cluster.md Outdated
@@ -335,6 +335,12 @@ the implication of this choice which are described both in the document above an

</aside>

### InfraCluster: Pausing
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please link this rule from the table up at L44 on this page (you can add this as a new line at the end)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added at the end, not sure if it should be underneath the other InfraCluster: <foo> lines though?

@theobarberbany theobarberbany force-pushed the pause-condition-update-contract branch from ccd3d15 to c6a8027 Compare October 7, 2024 10:53
@fabriziopandini
Copy link
Member

/lgtm
cc @sbueringer @chrischdi for a final check on the contract change (same will be applied to other contracts I guess)

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 43f75dcf90b7bc9e4b5722c10e2557495332b536

@theobarberbany theobarberbany mentioned this pull request Oct 8, 2024
Merged
@sbueringer
Copy link
Member

Thx!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2024
@k8s-ci-robot k8s-ci-robot merged commit fb691b8 into kubernetes-sigs:main Oct 10, 2024
16 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.9 milestone Oct 10, 2024
@nojnhuh nojnhuh mentioned this pull request Oct 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@fabriziopandini fabriziopandini fabriziopandini left review comments

@elmiko elmiko Awaiting requested review from elmiko

@jackfrancis jackfrancis Awaiting requested review from jackfrancis

@richardcase richardcase Awaiting requested review from richardcase

@stmcginnis stmcginnis Awaiting requested review from stmcginnis

@sbueringer sbueringer Awaiting requested review from sbueringer

Assignees

@fabriziopandini fabriziopandini

@JoelSpeed JoelSpeed

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/documentation Issues or PRs related to documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.9
Development

Successfully merging this pull request may close these issues.
5 participants
@theobarberbany @k8s-ci-robot @sbueringer @JoelSpeed @fabriziopandini